Skip to content

Fix memory leak in the timer #334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 18, 2024
Merged

Fix memory leak in the timer #334

merged 3 commits into from
Nov 18, 2024

Conversation

@shsms
Copy link
Contributor

@shsms shsms commented Nov 15, 2024

We were creating two futures, but only awaiting until the one of them
finished and not canceling the second.

Because reset() was never called, we were just creating more and
more futures that would never complete.

This change fixes this by canceling the second task immediately after
the first one is complete.

llucax and others added 3 commits September 25, 2024 11:39
@llucax
Clear release notes
5ab1279 
Signed-off-by: Leandro Lucarella <[email protected]>
@shsms
Fix memory leak in the timer
6a6907f 
We were creating two futures, but only awaiting until the one of them
finished and not canceling the second.

Because `reset()` was never called, we were just creating more and
more futures that would never complete.

This change fixes this by canceling the second task immediately after
the first one is complete.

Signed-off-by: Sahas Subramanian <[email protected]>
@shsms
Update release notes
b0eabec 
Signed-off-by: Sahas Subramanian <[email protected]>
@shsms shsms requested a review from a team as a code owner November 15, 2024 17:38
@github-actions github-actions bot added part:docs Affects the documentation part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`) labels Nov 15, 2024
@shsms shsms added the priority:high Address this as soon as possible label Nov 15, 2024
@shsms shsms self-assigned this Nov 15, 2024
@shsms shsms added this to the v1.2.1 milestone Nov 15, 2024
@shsms shsms added the type:bug Something isn't working label Nov 15, 2024
llucax
llucax approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh damn! Good catch! I wonder if we are getting any warnings in pytest we are ignoring, because pytest should report leaked futures.

@llucax
Copy link
Contributor

llucax commented Nov 18, 2024

I see we have some warnings about pending tasks in the tests:

Task was destroyed but it is pending!
task: <Task pending name='Task-393' coro=<Event.wait() running at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:213> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-395' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-397' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-399' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-402' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-405' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-406' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-409' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-410' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-413' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-415' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-418' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-419' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-421' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-428' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-430' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-439' coro=<sleep() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:637> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-440' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-442' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>
Task was destroyed but it is pending!
task: <Task pending name='Task-449' coro=<Event.wait() done, defined at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/locks.py:200> wait_for=<Future pending cb=[Task.task_wakeup()]> cb=[as_completed.<locals>._on_completion() at /home/luca/.local/share/uv/python/cpython-3.11.10-linux-x86_64-gnu/lib/python3.11/asyncio/tasks.py:602]>

Maybe we should make sure we don't get these anymore and start failing the CI job if we introduce new warnings...

@Marenz
Copy link
Contributor

Marenz commented Nov 18, 2024

Maybe we should make sure we don't get these anymore and start failing the CI job if we introduce new warnings...

I had code that would clean up the tasks afterwards, but one of you said it's unnecessary :P

@shsms shsms merged commit 224519c into frequenz-floss:v1.2.x Nov 18, 2024
16 checks passed
@llucax
Copy link
Contributor

llucax commented Nov 19, 2024

I had code that would clean up the tasks afterwards, but one of you said it's unnecessary :P

You lacked marketing. If you have said it was fixing a leak I would have paid more attention 🤣

Anyway, I learned quite a bit from this fantastic experience.

@llucax llucax mentioned this pull request Nov 19, 2024
Open
@shsms shsms deleted the don't-leak-tasks branch December 17, 2024 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@llucax llucax llucax approved these changes

@ela-kotulska-frequenz ela-kotulska-frequenz ela-kotulska-frequenz approved these changes

@daniel-zullo-frequenz daniel-zullo-frequenz daniel-zullo-frequenz approved these changes

Assignees

@shsms shsms

Labels

part:docs Affects the documentation part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`) priority:high Address this as soon as possible type:bug Something isn't working

Projects

None yet

Milestone

v1.2.1

Development

Successfully merging this pull request may close these issues.

5 participants

@shsms @llucax @Marenz @ela-kotulska-frequenz @daniel-zullo-frequenz